Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZSA Protocol: Transfer, Issuance and Burn: ZIPs 226, 227, and 230 #854

Merged
merged 22 commits into from
Nov 5, 2024

Conversation

vivek-arte
Copy link
Contributor

@vivek-arte vivek-arte commented Jun 11, 2024

This PR continues the discussion from the merged #778.

It contains any recent changes to the following ZIPs:

Note that the target issue for the ZSA ZIPs is #618

This pull request contains changes that resolve the following issues related to ZSAs:

This adds a field to the TxId digest to include the burn fields of
the transaction to the SIGHASH.
…ero number of issuance actions are present. (#54)

As described in the title, this adds to the transaction format to
require that the `ik` and `issueAuthSig` fields be present in the
transaction format if and only if there are a non-zero number of
issuance actions.
…he V7 transaction details (#55)

A quick summary of the changes here:
- The `assetDescSize` field in the issuance action description is
encoded as a `compactSize` type, as is standard across the specification
(and also allows for the use of the existing methods in the
implementation).
- The description of the notes used inside the issuance bundle were
explicitly serialized to remove ambiguity.
- The type of the `valueBurn` field was set to the standard `uint64`
type.
- The order of the Orchard bundle fields was adjusted to have the
binding signature after the burn fields.
@daira daira added the ZSAs label Jul 23, 2024
vivek-arte and others added 10 commits August 11, 2024 22:27
This PR catches our latest changes up with the rearranging of the
repository performed upstream.
…s map (#59)

This adds a map called `issued_assets` to the global state. This is
used to track ZSA balances in the shielded pool along the lines of ZIP
209.

This should resolve zcash#844.
This adds the symbolic link above in order to allow the
[qed-it.github.io/zips](qed-it.github.io/zips) website to work correctly
post the upstream repository restructure.
…escription string size (#61)

This rewrites the burn mechanism description in ZIP 226 in order to
remove some ambiguously defined terms, and improve the clarity of the
specification.

It also adds the rationale for the choice of 512 bytes for the maximum
length of the asset description string in ZIP 227, which resolves zcash#843
(along with the additions in the already merged #59)
)

This catches the zsa1 branch up to the latest upstream changes.
It also comments out a line from the render.yml file to allow the CI to
run correctly.
This adds the NU6 changes from upstream to our branch.
This updates the protocol spec references from 2023.4.0 [NU5] to
2024.5.1 [NU6], along with some minor section changes and renumbering.
It also fixes some old links in favour of updated ones.

This resolves zcash#751 for the present.
This makes the changes to align with the change in notation for ZIP
32 done in zcash#908.
zips/zip-0227.rst Outdated Show resolved Hide resolved
zips/zip-0227.rst Outdated Show resolved Hide resolved
- Add :math:`\mathsf{cm}` to the Merkle tree of note commitments.
- Increase the value of :math:`\mathsf{issued\_assets(AssetBase).balance}` by the value of the note, :math:`\mathsf{v}`.

- If :math:`\mathsf{finalize} = 1\!`, set :math:`\mathsf{issued\_assets(AssetBase).final}` to :math:`1` in the global state immediately after the block in which this transaction occurs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion among the ZIP Editors and I, we think this is inconsistent with other transaction processing which is sequential within a block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, we updated this in QED-it#75

@@ -353,7 +381,8 @@ The following is a list of rationale for different decisions made in the proposa
- bridging information for Wrapped Assets (chain of origin, issuer name, etc)
- information to be committed by the issuer, though not enforceable by the protocol.

- We require a check whether the :math:`\mathsf{finalize}` flag only has been set in a previous block rather than a previous transaction in the same block. In other words, we only update the :math:`\mathsf{previously\_finalized}`` set at the block boundary. This is in keeping with the current property which allows for a miner to reorder transactions in a block without changing the meaning, which we aim to preserve.
- We limit the size of the :math:`\mathsf{asset\_desc}` string to 512 bytes as it is a reasonable size to store metadata about the Asset, for example in JSON format.
- We require a check whether the :math:`\mathsf{finalize}` flag only has been set in a previous block rather than a previous transaction in the same block. In other words, we only update the :math:`\mathsf{issued\_assets}` map at the block boundary. This is in keeping with the current property which allows for a miner to reorder transactions in a block without changing the meaning, which we aim to preserve.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not correct that miners can currently reorder transactions within a block without changing the meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in QED-it#75

zips/zip-0230.rst Outdated Show resolved Hide resolved
zips/zip-0230.rst Outdated Show resolved Hide resolved
Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>
Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK for merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants